-
Notifications
You must be signed in to change notification settings - Fork 839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[EuiSuperSelect] Adding generated IDs to create accessible label and description #5364
Conversation
* Generated class attributes for aria-labelledby and aria-describedby * Updated snapshot tests * Adding CHANGELOG entry to main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one explanatory comment for reasoning why I removed an attribute.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5364/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5364/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this!
Two AXE errors from CI:
11:33:54 Errors on tp://localhost:9999/#/forms/compressed-forms
11:38:10 [button-name]: Ensures buttons have discernible text
11:38:10 Help: https://dequeuniversity.com/rules/axe/4.1/button-name?application=axe-puppeteer
11:38:10 Elements:
11:38:10 - .euiSuperSelectControl
11:38:10 Errors on tp://localhost:9999/#/elastic-charts/creating-charts
11:39:15 [button-name]: Ensures buttons have discernible text
11:39:15 Help: https://dequeuniversity.com/rules/axe/4.1/button-name?application=axe-puppeteer
11:39:15 Elements:
11:39:15 - .euiSuperSelectControl
11:39:15 2 accessibility errors
src/components/color_picker/color_palette_picker/color_palette_picker.tsx
Outdated
Show resolved
Hide resolved
* Removed `screenReaderId` from parent, passed directly to child component * Added screen reader text to the Color Palette Picker button * Updated snapshot tests
Preview documentation changes for this PR: https://eui.elastic.co/pr_5364/ |
src/components/color_picker/color_palette_picker/color_palette_picker.tsx
Outdated
Show resolved
Hide resolved
* Refactored to add `aria-hidden` to color bars * Added screen reader only text titles * Fixes an axe-core issue with buttons needing discernible text in SuperSelect * Added issue #5367 to CHANGELOG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last changes finished and tested. PR update incoming shortly.
src/components/color_picker/color_palette_picker/color_palette_picker.tsx
Outdated
Show resolved
Hide resolved
src/components/color_picker/color_palette_picker/color_palette_picker.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scope expanded a bit because of Color Palette Picker. I found a way to announce the Super Select listbox options consistently, and always have an accessible label for the button when the listbox is closed.
src/components/color_picker/color_palette_display/color_palette_display_gradient.tsx
Show resolved
Hide resolved
<span>{title}</span> | ||
</EuiScreenReaderOnly> | ||
<span | ||
aria-hidden="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This aria-hidden="true"
is to ensure the color blocks are ignored by screen readers, and the only accessible text for options is the title string.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5364/ |
@cchaos Added requested comment to both components. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5364/ |
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5364/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A changelog note I'd missed earlier, + a small code simplification
src/components/color_picker/color_palette_display/color_palette_display.tsx
Outdated
Show resolved
Hide resolved
Preview documentation changes for this PR: https://eui.elastic.co/pr_5364/ |
src/components/color_picker/color_palette_display/color_palette_display_fixed.tsx
Outdated
Show resolved
Hide resolved
src/components/color_picker/color_palette_picker/color_palette_picker.tsx
Show resolved
Hide resolved
Preview documentation changes for this PR: https://eui.elastic.co/pr_5364/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💟 All the a11y PR's!! LGTM, mostly as a code check, I didn't run through a screen-reader to confirm.
@cchaos I ran it through VoiceOver on MacOS. I'll pass a preview URL to Zuhair and see if he's got time to test with NVDA and JAWS for coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic, and with such a clean change set! This LGTM, tested functionality out in the PR's canary docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! I pulled the branch and tested with Voiceover (macOS BigSur). Awesome job!
@cchaos and myself went through the URLS with JAWS and NVDA. Both screen readers behaved as expected but we note the following minor issues:
@1Copenut will take out the number of items from the describedby , after which this should be complete. I will research why JAWS is not reading, and will open a future enhancement if a fix is needed on our end. |
I did? Was I sleep-walking? 🤣 |
Great find @zuhairmahd. I'm surprised the default verbosity ignores |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5364/ |
Summary
The
EuiSuperSelect
was flagging an error in axe-core where thediv[role="listbox"]
was missing an accessible label. I refactored it to include an accessible label and description. This improves screen reader UX by announcing a label and instructions for interacting with a keyboard. This PR closes #5292 and also closes #5367.title
attribute to SR-only components in Color Palette PickerProbably non-breaking
I thought about adding the breaking change tag, but feel we're safe making this change for a couple of reasons:
screenReaderId
prop is passed down from a class attribute that will always existEuiSuperSelect
is self-contained except for use inEuiColorPalettePicker
, which was updated to include the new prop.EuiColorPalettePicker
ended up needing atitle
attribute passed to child components so the<button>
that's rendered byEuiSuperSelectControl
always has an accessible label.Screen reader testing
Checklist
Props have proper autodocs and playground togglesChecked Code Sandbox works for any docs examples